Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integer #85

Merged
merged 38 commits into from
Sep 5, 2024
Merged

Integer #85

merged 38 commits into from
Sep 5, 2024

Conversation

desmonddak
Copy link
Contributor

Description & Motivation

Adds a very general booth encoding multiplier with important subcomponents: generalized booth encoding (2,4,8,16 currently supported), Wallace tree column compression (delay-driven with heuristic fixed delays for column compressors), and reusing the parallel prefix adder components for the final adder.

The multiplier base supports different widths of operands (so rectangular products) as well as signed and unsigned operands. Testing is quite exhaustive across signed, radix, width, skew (width difference).

Also, we added a multiply-accumulate which is simply introducing the third operand as part of the column compression.

Related Issue(s)

None.

Testing

Randomized testing to cover the wide space of radix, widths, skew, and signage. Using a full multiply-accumulate tester to test both MAC and multiply by using a zero-term for the third operand to keep test code crisp. Especially tricky has been compact sign extension (keeping the number of rows the same) across different widths and skews for signed and unsigned and this was tested exhaustively.

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

Changed existing multipliers to use an abstract Multiply module base class.

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

Documentation updated with the multiplier classes. Also updated the confapp generator to allow for web-based visualization.

@desmonddak desmonddak requested a review from mkorbel1 August 7, 2024 18:27
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

lib/src/arithmetic/booth.dart Outdated Show resolved Hide resolved
doc/components/multiplier.md Outdated Show resolved Hide resolved
lib/src/arithmetic/adder.dart Show resolved Hide resolved
lib/src/arithmetic/adder.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/booth.dart Outdated Show resolved Hide resolved
test/arithmetic/booth_test.dart Outdated Show resolved Hide resolved
test/arithmetic/compressor_test.dart Outdated Show resolved Hide resolved
test/arithmetic/compressor_test.dart Outdated Show resolved Hide resolved
test/arithmetic/multiplier_test.dart Outdated Show resolved Hide resolved
test/arithmetic/multiplier_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops meant to request changes, not approve

@desmonddak
Copy link
Contributor Author

desmonddak commented Aug 11, 2024

I think I am done with updates.
I added a couple of proposed extensions to LogicValue because one I am now using (firstOne()) in printing during evaluation.
There are a couple of open issues to discuss:

  1. A template for building input/output in a base Module and then deriving from that (the computeSum() issue). I think we should create a template in the documentation as this is a starting point for adding a component and it can be a bit confusing where to use this.<input> and late final, @protected, and get. A template and a few simple examples would go a long way.
  2. The yosys bug and need for sv2v for the schematic (I added the multiplier to the web configurator for example)
  3. My use of during-simulation evaluate code and how to structure that as an extension in test area only.
  4. The extensions I added -- are these ok for now
  5. A fair amount of restructured code based on your suggestions (folding in the sign extension into the constructor, getting rid of factory methods, etc.

@desmonddak
Copy link
Contributor Author

I have one more set of changes I want to make: generalizing the full multiply tests.

@desmonddak desmonddak requested a review from mkorbel1 August 12, 2024 15:05
README.md Show resolved Hide resolved
lib/src/utils.dart Outdated Show resolved Hide resolved
lib/src/utils.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new changes look good!

@desmonddak
Copy link
Contributor Author

I'm working on documentation now...
I'm not sure if this PR has gone on too long and we should reopen to clear out too much history and small changes/bugfixes.

@desmonddak desmonddak requested a review from mkorbel1 August 19, 2024 16:19
@desmonddak
Copy link
Contributor Author

A few areas for focusing comments:

  • documentation that I added -- especially how much I should reference args and describe calling (I provide examples).
  • multiplier_lib.dart -- I had to pull this into rohd_hcl.dart anyway to get documentation produced, so we may need to remove this file and merge into rohd_hcl.dart for simplicity.
  • tests: I run regressions but can trim them down more. I like this idea of having singleton tests and then the regression loops as when a bug surfaces it is easier to reconstruct using a singleton.
  • an idea I had: it would be nice to construct small examples that get run in the test area but get included in the .md files -- that way we don't end up with documentation that is incorrect in terms of calling conventions.
  • I'm tempted to create another PR to remove all this clumsy history.

@mkorbel1
Copy link
Contributor

I'm not sure if this PR has gone on too long and we should reopen to clear out too much history and small changes/bugfixes.

@desmonddak doesn't matter, this will all get squashed into one nice pretty commit when it merges :)

doc/components/adder.md Outdated Show resolved Hide resolved
lib/src/arithmetic/multiplier.dart Outdated Show resolved Hide resolved
doc/components/multiplier_components.md Outdated Show resolved Hide resolved
doc/components/multiplier_components.md Outdated Show resolved Hide resolved
doc/components/multiplier_components.md Outdated Show resolved Hide resolved
lib/src/arithmetic/sign_magnitude_adder.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/sign_magnitude_adder.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/parallel_prefix_operations.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/partial_product_generator.dart Outdated Show resolved Hide resolved
test/arithmetic/multiplier_encoder_test.dart Outdated Show resolved Hide resolved
@mkorbel1 mkorbel1 mentioned this pull request Aug 31, 2024
lib/rohd_hcl.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/addend_compressor.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/parallel_prefix_operations.dart Outdated Show resolved Hide resolved
lib/src/arithmetic/partial_product_generator.dart Outdated Show resolved Hide resolved
doc/components/multiplier_components.md Outdated Show resolved Hide resolved
lib/src/arithmetic/partial_product_generator.dart Outdated Show resolved Hide resolved
test/arithmetic/adder_test.dart Show resolved Hide resolved
Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@desmonddak desmonddak merged commit 063c66e into intel:main Sep 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants